NIFI-15931: Fetching parameter values in the Connector canvas.#11263
Conversation
|
reviewing... |
rfellows
left a comment
rfellows
left a comment
There was a problem hiding this comment.
Solid implementation overall — the in-memory graph walk is the right approach for connector-managed contexts, and the frontend effect/reducer/dialog wiring is clean. A few things to address before merging, noted inline.
| return fromGraph; | ||
| } | ||
|
|
||
| return parameterContextLookup.getParameterContext(sourceId); |
There was a problem hiding this comment.
resolveContainingParameterContext can return null here when both the in-memory graph walk and the lookup come up empty. The connector path in StandardNiFiServiceFacade passes ParameterContextLookup.EMPTY, whose getParameterContext unconditionally returns null. If a parameter carries a parameterContextId that is not reachable in the in-memory graph (e.g. data inconsistency after a reconciliation), this method returns null, and the caller on line 1625 immediately dereferences it with .getIdentifier(), producing an NPE.
Suggested fix — guard the return value:
final ParameterContext result = parameterContextLookup.getParameterContext(sourceId);
return result != null ? result : parameterContext;Falling back to parameterContext (treating the parameter as locally defined) is conservative and safe. Alternatively, throw an IllegalStateException with a descriptive message so the inconsistency surfaces clearly rather than crashing silently.
|
|
||
| // Connector-managed parameter contexts (and any contexts they inherit from) are not registered with the | ||
| // global flow's ParameterContextManager. The DTO factory now resolves a parameter's source context by | ||
| // walking the in-memory inheritance graph on the supplied context, so an empty lookup is sufficient here. |
There was a problem hiding this comment.
Nit: "now resolves" is patch-relative — it describes the code relative to what it used to do rather than its permanent contract. Comments should explain the invariant, not the change (those belong in the commit message).
Suggested rewrite:
// Connector-managed parameter contexts are not registered with the global ParameterContextManager,
// so the DAO-backed lookup would fail for any inherited parameter. The in-memory inheritance graph
// reachable from the supplied context is sufficient to resolve parameter source contexts for
// connector-managed flows, making an empty lookup safe here.| assertEquals(externalId, dto.getParameterContext().getId()); | ||
|
|
||
| verify(lookup).getParameterContext(externalId); | ||
| } |
There was a problem hiding this comment.
This test covers the fallback path when the lookup returns a valid context, but there is no companion test for the case where both the graph walk and the lookup return null — i.e. ParameterContextLookup.EMPTY, which is exactly what the connector path passes. Without it, the behavior of resolveContainingParameterContext when both paths come up empty is unspecified by the test suite, and the NPE at the call site (line 1625) would go undetected.
Suggested addition alongside the null-guard fix:
@Test
void testCreateParameterDtoFallsBackToCurrentContextWhenSourceNotReachableInGraphAndLookupIsEmpty() {
final String contextId = "context-1";
final String externalId = "context-external";
final ParameterContext parameterContext = createMockParameterContext(contextId, "context-1-name", Collections.emptyList());
final Parameter parameter = new Parameter.Builder()
.name("param-name")
.value("param-value")
.parameterContextId(externalId)
.build();
final DtoFactory dtoFactory = newDtoFactoryForParameters();
final ParameterDTO dto = dtoFactory.createParameterDto(
parameterContext, parameter, mock(RevisionManager.class), ParameterContextLookup.EMPTY);
// Source context not found in graph or lookup; falls back to treating as local
assertFalse(dto.getInherited());
assertEquals(contextId, dto.getParameterContext().getId());
}(Exact assertion depends on the chosen fallback behavior, but the test is needed either way to pin the contract.)
| assertEquals(missingId, dto.getParameterContext().getId()); | ||
|
|
||
| verify(lookup).getParameterContext(missingId); | ||
| } |
There was a problem hiding this comment.
The cycle test covers mutual recursion (child → parent → child), but the visited set in findInheritedParameterContext also guards against redundant traversal in diamond inheritance (A inherits B and C, both inherit D). That second behavior is untested. Without it, a future change that accidentally removes or weakens the visited-set guard would not be caught.
Suggested addition:
@Test
void testCreateParameterDtoResolvesSourceContextFromDiamondInheritanceGraph() {
final String contextAId = "context-a";
final String contextBId = "context-b";
final String contextCId = "context-c";
final String contextDId = "context-d";
final ParameterContext contextD = createMockParameterContext(contextDId, "context-d-name", Collections.emptyList());
final ParameterContext contextB = createMockParameterContext(contextBId, "context-b-name", List.of(contextD));
final ParameterContext contextC = createMockParameterContext(contextCId, "context-c-name", List.of(contextD));
final ParameterContext contextA = createMockParameterContext(contextAId, "context-a-name", List.of(contextB, contextC));
final Parameter parameter = new Parameter.Builder()
.name("param-name")
.value("param-value")
.parameterContextId(contextDId)
.build();
final DtoFactory dtoFactory = newDtoFactoryForParameters();
final ParameterDTO dto = dtoFactory.createParameterDto(
contextA, parameter, mock(RevisionManager.class), ParameterContextLookup.EMPTY);
assertTrue(dto.getInherited());
assertEquals(contextDId, dto.getParameterContext().getId());
}| errorContext: ErrorContextKey.CONNECTOR_CANVAS | ||
| }) | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
The dedup key in loadConnectorParameterContextOnLoadSuccess$ is (connectorId, processGroupId) — both fields must change (or one must change) to pass the distinctUntilChanged predicate. This test covers the PG-changes case, but the symmetric case — same PG id, different connector — is missing. Without it, someone could simplify the predicate to check only processGroupId and no test would fail, even though navigating between two connectors with the same root PG id would silently skip refetching the parameter context.
Suggested addition:
it('should dispatch again when the connector changes even if the process group id is the same', async () => {
const { effects, actions$ } = await setup();
actions$(
of(
flowSuccessAction('connector-123', 'pg-abc'),
flowSuccessAction('connector-456', 'pg-abc')
)
);
const emitted = await firstValueFrom(effects.loadConnectorParameterContextOnLoadSuccess$.pipe(toArray()));
expect(emitted).toEqual([
loadConnectorParameterContext({
connectorId: 'connector-123',
processGroupId: 'pg-abc',
errorContext: ErrorContextKey.CONNECTOR_CANVAS
}),
loadConnectorParameterContext({
connectorId: 'connector-456',
processGroupId: 'pg-abc',
errorContext: ErrorContextKey.CONNECTOR_CANVAS
})
]);
});
|
Also, I didn't see another JIRA anywhere to account for the same behavior for Controller Services used by connectors. If they use parameters, we don't get the same tooltip resolution: 
|
| distinctUntilChanged( | ||
| (previous, current) => | ||
| previous.connectorId === current.connectorId && previous.processGroupId === current.processGroupId | ||
| ), |
There was a problem hiding this comment.
NgRx effects are application-level singletons — they're created once when the module initializes and run forever. They are never destroyed or recreated when a component mounts/unmounts.
The distinctUntilChanged inside the effect accumulates its previous value for the entire app lifetime. This results in the parameter-context lookup to only get loaded when the browser loads the connector canvas directly (deep link). If you navigate to it from the connector listing, the parameters don't get loaded.
The store resets (resetConnectorCanvasState in ngOnDestroy) clear parameterContext from the state, but that does nothing to clear the effect's internal previous reference. From the effect's perspective the (connectorId, processGroupId) pair hasn't changed, so it silently suppresses the re-fetch.
The fix
The effect needs to restart its inner distinctUntilChanged whenever the canvas state is reset. The idiomatic RxJS pattern for this is startWith + switchMap on the reset action:
loadConnectorParameterContextOnLoadSuccess$ = createEffect(() =>
this.actions$.pipe(
ofType(ConnectorCanvasActions.resetConnectorCanvasState),
startWith(null), // kick off the first inner subscription immediately
switchMap(() => // each reset discards the old inner sub (and its stale distinctUntilChanged state) and starts fresh
this.actions$.pipe(
ofType(
ConnectorCanvasActions.loadConnectorFlowSuccess,
ConnectorControllerServicesActions.loadConnectorControllerServicesSuccess
),
map((action) => { /* normalize */ }),
filter((target): target is ... => target.processGroupId != null),
distinctUntilChanged(
(previous, current) =>
previous.connectorId === current.connectorId &&
previous.processGroupId === current.processGroupId
),
map((target) => ConnectorCanvasActions.loadConnectorParameterContext(target))
)
)
)
);When resetConnectorCanvasState fires (component ngOnDestroy), switchMap cancels the inner observable — clearing the stale previous — and subscribes to a fresh one. The next loadConnectorFlowSuccess is then the first emission in that new inner stream, so distinctUntilChanged lets it through.
The startWith(null) is essential: without it, the effect wouldn't start listening for load-success actions until the first reset fires.
This was actually caused by the other iterms you called out. It's been addressed in my most recent commit. |
2 participants
Summary
JIRA: NIFI-15931
Surfaces parameter values inline in the Connector canvas so property tables and the codemirror parameter tip can resolve
#{param}references without leaving the canvas. Read-only by design: values render in tooltips, but the "Go to Parameter" affordance is hidden because connector-managed parameter contexts are not navigable.Description
Backend
GET /connectors/{connectorId}/flow/process-groups/{processGroupId}/parameter-contextonConnectorResource. RequiresREADon the connector, returns aParameterContextEntityfor the PG's bound context, or204 No Contentwhen no context is bound. Sensitive values are masked via the existingDtoFactorypath.NiFiServiceFacade#getConnectorParameterContextand itsStandardNiFiServiceFacadeimplementation look up theConnectorNode, locate the targetProcessGroup, and return its boundParameterContext.DtoFactory#createParameterDtonow walks the in-memory inheritance graph of the suppliedParameterContextto populatecontainingParameterContext(with cycle protection), falling back to the suppliedParameterContextLookuponly when no in-memory match is found. This is required because connector-managed parameter contexts are not registered in the globalParameterContextDAO, so the previous lookup-only path returned "Unable to find Parameter Context …" for any inherited parameter on a connector PG.Frontend
ConnectorService.getConnectorParameterContext(connectorId, processGroupId)wraps the new endpoint, mapping204tonullso the caller can distinguish "no context" from a load failure.loadConnectorParameterContext/Success/Failure) andparameterContextslice onConnectorCanvasState. Selectors expose the current value to dialogs.loadConnectorParameterContextOnLoadSuccess$, listens to bothloadConnectorFlowSuccessandloadConnectorControllerServicesSuccess, normalizes their payloads, filters out nullprocessGroupIds, and dedupes viadistinctUntilChangedon(connectorId, processGroupId). This covers the deep-link case where the user lands directly on/connectors/:id/canvas/:pgId/controller-services(the CS route is a sibling of the canvas route, not a child, so it does not activate the canvas component) without firing duplicate fetches when polling or the canvas → CS transition refires within the same PG.errorContext: ErrorContextKeychosen by the trigger (CONNECTOR_CANVASfor canvas,CONTROLLER_SERVICESfor CS-listing) soloadConnectorParameterContext$can surface failures on the page the user is actually looking at viaaddBannerError.bindConnectorParameterContext(store, teardown$, apply)wires the current parameter context onto the read-onlyEditProcessorandEditControllerServicedialogs for the lifetime ofdialogRef.afterClosed().goToParameteris intentionally left undefined; the property table now hides the "Go to Parameter" menu item whenever the callback is missing (property-table.component.ts).@Input() goToParameter?: (parameter: string) => voidis now optional onEditProcessorandEditControllerServiceto match.property-value-tiptemplate and the codemirrorparameter-tiptemplate render explicit empty-state copy ("No value set" / "Empty string set") so users can tell unset parameters from values that simply weren't fetched. Bothnullandundefinedare handled because the wire format prunes nulls.connectorId(in addition toprocessGroupId) changes, so navigating between connectors doesn't briefly leak stale shapes.Test fixture
pages/connectors/testing/parameter-context-fixture.tscentralizes theas unknown as ParameterContextEntitycast for test fixtures.Tests
DtoFactoryTest: 7 new cases covering null/matching IDs, direct and transitive inheritance, fallback to lookup, cyclic inheritance graphs, and sensitive value masking.StandardNiFiServiceFacadeTest: happy path returning the entity, null when no context is bound, andResourceNotFoundExceptionwhen the PG cannot be located.TestConnectorResource: 200 / 204 /AccessDeniedExceptioncases for the new endpoint.ConnectorService(200 vs 204),connectorCanvasReducerparameter context handling and connector-id reset behavior,ConnectorCanvasEffects(load on canvas + CS deep link, polling/transition dedupe, failure routing per trigger),ConnectorControllerServicesEffectsparameter-context binding in the read-only dialog, andproperty-tablecanGoToParameterwith/without callback. Tooltip rendering fornull/undefined/''/non-empty across bothproperty-value-tipandparameter-tip.